Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce Body Limit Middleware using stream #2103

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

yusukebe
Copy link
Member

@yusukebe yusukebe commented Jan 27, 2024

This is an implementation of the feature proposed in #2077 using stream.

How to use

Simply specify the maximum size.

app.post(
  '/hello',
  bodyLimit({
    maxSize: 15 * Unit.b, // byte,
    onError: (c) => {
      return c.text('oveflow :(', 413)
    },
  }),
  (c) => {
    return c.text('pass :)')
  }
)

Using stream

The problem discussed in #2077 that a body must be read at a time is solved by using stream.

c.req.bodyCache

It uses c.req.bodyCache used by the Validator implementation. By setting the loaded ArrayBuffer to c.req.bodyCache.arrayBuffer, such as c.req.json() will use that cached body from then on.

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

@yusukebe yusukebe added the v4 label Jan 29, 2024
@yusukebe yusukebe removed the v4 label Feb 5, 2024
Base automatically changed from v4 to main February 9, 2024 05:53
@yusukebe yusukebe added the v4.1 label Feb 21, 2024
@rafaell-lycan
Copy link

Express has an effortless way to customize its body limit, defaulting to 100kb max.

…2109)

* fix(body-limit): fix typo

* feat(body-limit): Replace `c.req.raw` with body limit middleware proxy

* refactor(body-limit): we can trust content-length header

* fix(body-limit): call controller.error instead of throwing an error

* test(body-limit): add test for ReadableStream body

* chore: denoify

* refactor(middleware/body-limit): throw HTTPException instead of retuning c.text()
@yusukebe
Copy link
Member Author

Hey @usualoma

Could you review this merged code again?

@usualoma
Copy link
Member

usualoma commented Feb 27, 2024

Do we need data called Unit?

As a matter of fact, to me, the following statement seems cognitively loaded. Of course it is not incomprehensible, and I think it is wonderful that it comes with a type. But later, when you take a quick look at the code, what is Unit?" is not so obvious and requires knowledge of body-limit.

impot { bodyLimit, Unit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: 15 * Unit.mb,
})

I find it less cognitively demanding to write the following rather than using utility type.

impot { bodyLimit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: 15 * 1024 * 1024, // 15mb
})

Alternatively, if we need to provide a way to specify using units, I feel the following would be better

impot { bodyLimit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: '15mb',
})
diff --git a/src/middleware/body-limit/index.test.ts b/src/middleware/body-limit/index.test.ts
index 058ffc77..7be5c41b 100644
--- a/src/middleware/body-limit/index.test.ts
+++ b/src/middleware/body-limit/index.test.ts
@@ -1,5 +1,6 @@
 import { Hono } from '../../hono'
-import { Unit, bodyLimit } from '.'
+import { bodyLimit } from '.'
+import type { Size } from './index'
 
 const GlobalRequest = globalThis.Request
 globalThis.Request = class Request extends GlobalRequest {
@@ -35,7 +36,7 @@ describe('Body Limit Middleware', () => {
 
   beforeEach(() => {
     app = new Hono()
-    app.use('*', bodyLimit({ maxSize: 15 * Unit.b }))
+    app.use('*', bodyLimit({ maxSize: '15b' }))
     app.get('/', (c) => c.text('index'))
     app.post('/body-limit-15byte', async (c) => {
       return c.text(await c.req.raw.text())
@@ -112,45 +113,49 @@ describe('Body Limit Middleware', () => {
     })
   })
 
-  describe('custom error handler', () => {
+  describe('Size', () => {
+    it('should accept string with unit', () => {
+      expectTypeOf<Size>('1b').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1kb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1mb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1gb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1tb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1pb').toEqualTypeOf<Size>('1b')
+
+      expectTypeOf<Size>('1.5gb').toEqualTypeOf<Size>('1b')
+    })
+    it('should accept number', () => {
+      expectTypeOf<Size>(1).toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>(1.1).toEqualTypeOf<Size>('1b')
+    })
+  })
+
+  describe('maxSize with string', () => {
     beforeEach(() => {
       app = new Hono()
-      app.post(
-        '/text-limit-15byte-custom',
-        bodyLimit({
-          maxSize: 15 * Unit.b,
-          onError: (c) => {
-            return c.text('no', 413)
-          },
-        }),
-        (c) => {
-          return c.text('yes')
-        }
-      )
     })
 
-    it('should return the custom error handler', async () => {
-      const res = await app.request(
-        '/text-limit-15byte-custom',
-        buildRequestInit({ body: exampleText2 })
+    test.each`
+      maxSize  | size
+      ${'15b'} | ${15}
+      ${'1kb'} | ${1 * 1024}
+      ${'1mb'} | ${1 * 1024 ** 2}
+    `('should accept $maxSize as byte unit', async ({ maxSize, size }) => {
+      app.post(
+        '/hello',
+        bodyLimit({
+          maxSize,
+        }),
+        (c) => c.text('pass :)')
       )
 
+      let res = await app.request('/hello', buildRequestInit({ body: 'a'.repeat(size) }))
+      expect(res).not.toBeNull()
+      expect(res.status).toBe(200)
+
+      res = await app.request('/hello', buildRequestInit({ body: 'a'.repeat(size + 1) }))
       expect(res).not.toBeNull()
       expect(res.status).toBe(413)
-      expect(await res.text()).toBe('no')
     })
   })
 })
-
-describe('Unit', () => {
-  it('should return the correct size', () => {
-    let beforeSize = 1 / 1024
-
-    for (let i = 0, keys = Object.keys(Unit), len = keys.length; i < len; i++) {
-      // @ts-expect-error: <safe access>
-      const size = Unit[keys[i]]
-      expect(size === beforeSize * 1024).toBeTruthy()
-      beforeSize = size
-    }
-  })
-})
diff --git a/src/middleware/body-limit/index.ts b/src/middleware/body-limit/index.ts
index 929de058..77610ca5 100644
--- a/src/middleware/body-limit/index.ts
+++ b/src/middleware/body-limit/index.ts
@@ -4,9 +4,15 @@ import type { MiddlewareHandler } from '../../types'
 
 const ERROR_MESSAGE = 'Payload Too Large'
 
+const metricPrefixes = ['', 'k', 'm', 'g', 't', 'p'] as const
+type Unit = `${typeof metricPrefixes[number]}b`
+
+export type Size = number | `${number}${Unit}`
+const sizeRe = new RegExp(`(.*?)(${metricPrefixes.join('|')})b$`)
+
 type OnError = (c: Context) => Response | Promise<Response>
 type BodyLimitOptions = {
-  maxSize: number
+  maxSize: Size
   onError?: OnError
 }
 
@@ -17,6 +23,15 @@ class BodyLimitError extends Error {
   }
 }
 
+const calculateSize = (maxSize: Size): number => {
+  if (typeof maxSize === 'number') {
+    return maxSize
+  }
+
+  const [, size, prefix] = maxSize.match(sizeRe) as [unknown, string, typeof metricPrefixes[number]]
+  return parseFloat(size) * 1024 ** metricPrefixes.indexOf(prefix)
+}
+
 /**
  * Body Limit Middleware
  *
@@ -25,7 +40,7 @@ class BodyLimitError extends Error {
  * app.post(
  *  '/hello',
  *  bodyLimit({
- *    maxSize: 15 * Unit.b,
+ *    maxSize: "15mb",
  *    onError: (c) => {
  *      return c.text('overflow :(', 413)
  *    }
@@ -45,7 +60,7 @@ export const bodyLimit = (options: BodyLimitOptions): MiddlewareHandler => {
       })
       throw new HTTPException(413, { res })
     })
-  const maxSize = options.maxSize
+  const maxSize = calculateSize(options.maxSize)
 
   return async function bodyLimit(c, next) {
     if (!c.req.raw.body) {
@@ -94,13 +109,3 @@ export const bodyLimit = (options: BodyLimitOptions): MiddlewareHandler => {
     }
   }
 }
-
-/**
- * Unit any
- * @example
- * ```ts
- * const limit = 100 * Unit.kb // 100kb
- * const limit2 = 1 * Unit.gb // 1gb
- * ```
- */
-export const Unit = { b: 1, kb: 1024, mb: 1024 ** 2, gb: 1024 ** 3, tb: 1024 ** 4, pb: 1024 ** 5 }

@usualoma
Copy link
Member

Personally, I have the above opinion about the current specifications of Unit.
But, well, it doesn't mean that it is no good. If the current specifications of Unit are good as they are, then I think it is good regarding other points.

@usualoma
Copy link
Member

(A few updated implementation examples)

@yusukebe
Copy link
Member Author

yusukebe commented Mar 1, 2024

Thanks, @usualoma !

As for Unit, I think removing it might be a good idea. I agree with the following opinion.

I find it less cognitively demanding to write the following rather than using utility type.

impot { bodyLimit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: 15 * 1024 * 1024, // 15mb
})

@yusukebe
Copy link
Member Author

yusukebe commented Mar 3, 2024

All works are finished!

@EdamAme-x If you are okay, I'll merge this PR to the "next" with you as a co-author!

@usualoma
Copy link
Member

usualoma commented Mar 3, 2024

@yusukebe
Copy link
Member Author

yusukebe commented Mar 3, 2024

@usualoma

You are right. Updated!

@yusukebe
Copy link
Member Author

yusukebe commented Mar 4, 2024

Hi @EdamAme-x! Now, I'll merge this into the "next". Thanks a lot!

@yusukebe yusukebe merged commit 2a9c9a1 into main Mar 4, 2024
10 checks passed
@yusukebe yusukebe deleted the feat/body-limit-stream branch March 4, 2024 12:36
@yusukebe yusukebe restored the feat/body-limit-stream branch March 4, 2024 12:37
yusukebe added a commit that referenced this pull request Mar 4, 2024
@yusukebe
Copy link
Member Author

yusukebe commented Mar 4, 2024

Oops. Mistook. Merged into the main. Reverting.

@yusukebe yusukebe deleted the feat/body-limit-stream branch March 4, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants